Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix random crashes using xija_gui_fit on Linux, change limit names for annotation, use CxoTime #108

Merged
merged 20 commits into from
Apr 1, 2021

Conversation

jzuhone
Copy link
Collaborator

@jzuhone jzuhone commented Mar 18, 2021

Description

This PR resolves an issue where xija_gui_fit would seemingly randomly crash on Linux when manipulating plots. The exact reason is unknown, because I could never seem to produce a traceback of any kind, but many of the plot objects (such as Figure and Axes were being re-created every time the data itself needed an update due to a new model fit, etc. This PR implements the changing of the data on the plot itself (for several plot types) instead of re-creating the plot objects with every update, which appears to fix the problem.

This PR also does the following:

  • Uses CxoTime instead of Chandra.Time
  • annotate_limits has been moved from being a method of the XijaModel class to being a function in the xija_gui_fit app, since this is the only place it's used
  • Removes old attempt at import of Chandra.taco which is a ska2 remnant
  • Implement the names for limit values suggested in when plotting them Add top-level "limits" key in xija JSON specification #70 (comment)

Testing

  • Passes unit tests on macOS
  • Functional testing on Linux and macOS (with independent testing from @taldcroft)

Fixes issue #105.

@jzuhone
Copy link
Collaborator Author

jzuhone commented Mar 18, 2021

I don't see crashes anymore, but I want @Gregg140 to confirm.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of nitpicky style comments that are optional. I cannot say that I reviewed the logic changes in detail, but I did do a functional test to confirm that the fit method fix worked, and ran xija_gui_fit and went through the basic API functionality with no problems. I also ran the unit tests and they pass.

The limit naming looks good, thanks for going along with the suggested naming convention. With this then we can update all the relevant xija models with limits?

return []
lines = []
draw_line = getattr(ax, f'ax{dir}line')
if 'planning.data_quality.high.acisi' in limits:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could all be factored into an enclosed function draw_line(limit_name, linestyle, color) which has limits in scope so you just have a series of calls like:

for limit_name, linestyle, color in [
    ('planning.data_quality.high.acisi', '-.', 'blue'),
    (...)]:
    draw_line(limit_name, linestyle, color)

self.canvas = canvas
self.ax1 = self.canvas.fig.add_subplot(121)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 1, 2, 1 in new code.

@Gregg140
Copy link

The good news is that I could not make it crash.

I detected a couple of issues in my tests:

  1. When I clicked on "Show Limits" for both the main data v time plot and the histogram I never saw the limit lines. Example:

Histo_show_lim

  1. After doing some fitting, I asked for the histogram to be re-drawn and I got the following picture.

Pad_Plus_y

The issue is that on the right hand plot I can't tell if the data continues higher than ~13 or it I'm seeing the precise top of the data. A little padding would be good so that I'm sure I'm seeing the top of the histogram.

Example of tests performed:

xija_gui_fit dpa_model_spec.json --days 365 --stop 2021:079:00:00:00.00 &

  1. Delete dpa0 solar heat plot

  2. Add 1dpamzt resid_time plot

  3. Clicked on show limits
    - Limits did not appear on the model v data plot

  4. Click Histogram

  5. Histogram plot: Show limits

    - Limits did not appear on left hand plot

  6. Mask FMT1

  7. Mask Radzones

  8. Redraw Histo

  9. Histo limits off

  10. Mask FMT1 off

  11. Mask Radzones off

  12. fit tau, ampl, bias

  13. Mask FMT1, radzones, show limits

  14. Unmask FMT!, radzones, show limits

  15. Redraw

  16. Annotate line

  17. Click on main plot to change annotation values

  18. Redraw histo

  19. Close histo

  20. Open histo

  21. Redraw histo

  22. add resid_data plot

  23. Fit P's

  24. Redraw Histo

    Need to pad the +y axis

@jzuhone
Copy link
Collaborator Author

jzuhone commented Mar 30, 2021

@Gregg140 this should be fixed now.

@Gregg140
Copy link

Crashed when running through the tests I listed. But I winnowed it down to this:

  1. fit tau, ampl, bias

  2. Click on histogram

Traceback (most recent call last):
File "/data/acis/ska_pkg/xija/xija/gui_fit/app.py", line 932, in make_histogram
self.hist_window = HistogramWindow(self.model, self.hist_msids)
File "/data/acis/ska_pkg/xija/xija/gui_fit/plots.py", line 275, in init
self.make_plots()
File "/data/acis/ska_pkg/xija/xija/gui_fit/plots.py", line 336, in make_plots
self.update_plots()
File "/data/acis/ska_pkg/xija/xija/gui_fit/plots.py", line 416, in update_plots
self.ax2.set_ylim(0.0, hist.max()+1)
File "/data/acis/miniconda3/envs/ska-dev/lib/python3.8/site-packages/matplotlib/axes/_base.py", line 3578, in set_ylim
top = self._validate_converted_limits(top, self.convert_yunits)
File "/data/acis/miniconda3/envs/ska-dev/lib/python3.8/site-packages/matplotlib/axes/_base.py", line 3101, in _validate_converted_limits
raise ValueError("Axis limits cannot be NaN or Inf")
ValueError: Axis limits cannot be NaN or Inf

@jzuhone
Copy link
Collaborator Author

jzuhone commented Mar 30, 2021

Thanks for catching that--fixed now.

@jzuhone
Copy link
Collaborator Author

jzuhone commented Mar 30, 2021

@taldcroft I added your suggestions also.

Copy link

@Gregg140 Gregg140 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based upon my limited testing of the tool I approve this change.

@jzuhone
Copy link
Collaborator Author

jzuhone commented Apr 1, 2021

@taldcroft this is ready to go.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants